refactor: Support batch export models as views #23052
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tomasfarias
commented
Jun 18, 2024
Comment on lines
143
to
129
| @pytest_asyncio.fixture(scope="module", autouse=True) | ||
| async def create_batch_export_views(clickhouse_client, django_db_setup): |
There was a problem hiding this comment.
Apparently clickhouse migrations don't really run in tests.
tomasfarias
commented
Jun 18, 2024
| @@ -1,6 +1,5 @@ | |||
| import datetime as dt | |||
| import json | |||
| import uuid | |||
There was a problem hiding this comment.
Every change on this file is just cleaning up.
tomasfarias
commented
Jun 18, 2024
| @@ -357,6 +359,23 @@ def stream_query_as_arrow( | |||
| with pa.ipc.open_stream(pa.PythonFile(response.raw)) as reader: | |||
| yield from reader | |||
|
|
|||
| async def astream_query_as_arrow( | |||
There was a problem hiding this comment.
Not in use yet, will use for person model exports.
tomasfarias
commented
Jun 18, 2024
| # without battle testing it first. | ||
| # There are already changes going out to the queries themselves that will impact events in a | ||
| # positive way. So, we can come back later and drop this block. | ||
| for record_batch in iter_records(client, team_id=team_id, is_backfill=is_backfill, **parameters): |
There was a problem hiding this comment.
See TODO above: For now, keeping old functionality intact for safety. Will release person model first, test, and then switch events.
tomasfarias
force-pushed
the
feat/add-schema-selector-to-batch-exports-ui
branch
from
a2744da to
4f182c7
Compare
tomasfarias
commented
Jun 18, 2024
| @@ -77,78 +77,20 @@ def get_timestamp_field(is_backfill: bool) -> str: | |||
| return timestamp_field | |||
|
|
|||
|
|
|||
| async def get_rows_count( | |||
There was a problem hiding this comment.
We don't count rows anymore, so this function is unused. Cleaning up.
Base automatically changed from
feat/add-schema-selector-to-batch-exports-ui
to
master
June 18, 2024 18:04
tomasfarias
force-pushed
the
refactor/support-for-batch-export-model-views
branch
from
ecb38cb to
dc9535f
Compare
fuziontech
reviewed
Jun 19, 2024
posthog/batch_exports/sql.py
Outdated
Comment on lines
51
to
53
| PREWHERE | ||
| COALESCE(events.inserted_at, events._timestamp) >= {interval_start:DateTime64} | ||
| AND COALESCE(events.inserted_at, events._timestamp) < {interval_end:DateTime64} |
At this point, inserted_at should always be set for all batch exports. Only historical exports require _timestamp, but backfills have already been switched over to query based on timestamp, so they also do not need to check for inserted_at/_timestamp. Removing the colaesce and using only inserted_at reduces the size of the data CH has to fetch by half.
tomasfarias
force-pushed
the
refactor/support-for-batch-export-model-views
branch
from
6769736 to
2eabe0c
Compare
fuziontech
approved these changes
Jun 23, 2024
| ) | ||
| FORMAT ArrowStream |
There was a problem hiding this comment.
| yield record_batch | ||
| return | ||
|
|
||
| async for record_batch in client.astream_query_as_arrow(view, query_parameters=parameters): |
timgl
pushed a commit
that referenced
this pull request
Jun 27, 2024
* refactor: Update metrics to fetch counts at request time * fix: Move import to method * fix: Add function * feat: Custom schemas for batch exports * feat: Frontend support for model field * fix: Clean-up * fix: Add missing migration * fix: Make new field nullable * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * fix: Bump migration number * fix: Bump migration number * refactor: Update metrics to fetch counts at request time * fix: Actually use include and exclude events * refactor: Switch to counting runs * refactor: Support batch export models as views * fix: Merge conflict * fix: Quality check fixes * refactor: Update metrics to fetch counts at request time * fix: Move import to method * fix: Add function * fix: Typing fixes * feat: Custom schemas for batch exports * feat: Frontend support for model field * fix: Clean-up * fix: Add missing migration * fix: Make new field nullable * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (1) * fix: Bump migration number * fix: Clean-up unused code * chore: Clean-up unused function and tests * fix: Clean-up unused function * fix: HTTP Batch export default fields * fix: Remove test case on new column not present in base table * chore: Clean-up unused functions and queries * fix: Only run extra clickhouse queries in batch exports tests * refactor: Remove coalesce and use only inserted_at in queries At this point, inserted_at should always be set for all batch exports. Only historical exports require _timestamp, but backfills have already been switched over to query based on timestamp, so they also do not need to check for inserted_at/_timestamp. Removing the colaesce and using only inserted_at reduces the size of the data CH has to fetch by half. * fix: Remove deprecated test * fix: Add person_id to person model and enforce ordering * refactor: Also add version column --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
This PR sets the ground work to support person batch exports by introducing a set of parametrized views that will serve as the basis of batch exports moving forward. This new set of views includes a view for the persons model. Using parametrized views over the old approach has the benefit of being easier to parse the query logic when it's laid out in plain SQL (ideally this should be SQL files, but our migration system works with Python files...).
Changes
Introduce new parametrized views for person and events batch exports.
GROUP BYinstead ofDISTINCT. This should perform better and use less memory (in particular withoptimize_aggregation_in_orderenabled).PREWHEREclause for filtering first oninserted_at. Once again, this should result in better performance.timestampandinserted_atfilters. Should behave the same as before, but laid out in plain SQL instead of composed via Python formatting.Introduce new methods to iterate over records asynchronously.
pyarrow.RecordBatch.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?